Add expose_ports config option to publish extra container ports - #433
Open
skyrpex wants to merge 3 commits into
Open
Add expose_ports config option to publish extra container ports#433skyrpex wants to merge 3 commits into
skyrpex wants to merge 3 commits into
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Adds an expose_ports config option to publish extra container ports (motivated by DNS on 53); the grammar parsing, skip-vs-warn merge logic, and UDP preflight carve-out are all carefully reasoned and well covered by unit + integration tests.
- thought: on
internal/config/containers.go— the bare-port → tcp+udp expansion is the right default for DNS, but it also meansexpose_ports = [8080]for a TCP-only service publishes an unused8080/udpbinding. Harmless and documented, just worth keeping in mind if the docs example ever grows past DNS. - thought (non-blocking): on
internal/runtime/docker.go— expose_ports inherit the containerBindHost, so with a non-loopbackBindHost(e.g.0.0.0.0) a[53]entry makes the emulator DNS resolver reachable on the LAN. Consistent with every other published port, so not a behavior change — but worth a line in the docs since 53 is the headline use case. - praise: the
mergeExposePortssilent-skip (redundant entry) vs. warn (automatic mapping overrides the request) distinction and the TCP-only preflight carve-out for UDP are exactly the subtle cases that break silently in less careful implementations — nicely pinned by tests.
Automated review on behalf of @gtsiolis.
Generated by Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
DEVX-994 asks for a way to expose extra container ports (e.g. port 53 for the emulator's DNS server,
--host-dnsin v1) beyond what lstk publishes by default. Per the ticket's own suggested approach, this adds a genericexpose_portsconfig option rather than a DNS-specific flag.Changes
expose_portsconfig option to publish additional ports on the emulator container.For docs
New config knob, no CLI flag. Add a section under the
config.tomlreference (nearvolumes/container_name):--host-dns).expose_portsinside a[[containers]]block.53→ publishes port 53 on the host for both TCP and UDP (a bare number covers both, since DNS needs both)."5354:5353/udp"→host:container/protocol— host port 5354 maps to container port 5353, UDP only."53/tcp"→ protocol-only entry, host port defaults to the same number.lstk startfails outright (unlike some other lstk ports, this one won't silently skip and continue).expose_portsentries bind to the same host address as the emulator's other published ports. With a non-loopback bind address (e.g.0.0.0.0), a[53]entry makes the emulator's DNS resolver reachable from the local network — consistent with every other published port, but worth being aware of since DNS is the headline use case.expose_portsto change ports lstk already manages (4566, 443, the 4510-4559 range) — those are fixed regardless of what you put here.Review
Human review advised — new user-facing config surface, and the bare-port-expands-to-tcp+udp behavior is a judgment call worth a second look.
Tests
internal/container.test/integration/start_test.go: default-ports-unchanged, expose+remap+protocol edge cases, port-already-taken, and config validation.make test,make lintpass.Closes DEVX-994